Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

RFC2136 fixes #6858

Merged
merged 6 commits into from Aug 21, 2018
Merged

RFC2136 fixes #6858

merged 6 commits into from Aug 21, 2018

Conversation

Habbie
Copy link
Member

@Habbie Habbie commented Aug 16, 2018 •
edited

Short description

This fixes:

  • when updating a record in a child zone that also exists in the parent zone, we would incorrectly apply the update to the parent zone
  • two of the lookup calls would not always be followed by a full get consumption cycle
  • sort -V is a non-portable GNUism

This needs:

  • a test for the child/parent bug

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)
  • checked that this code was merged to master

@Habbie Habbie changed the title [WIP] RFC2136 fixes RFC2136 fixes Aug 16, 2018
@Habbie Habbie added this to the auth-4.1.x milestone Aug 16, 2018
@Habbie Habbie changed the title RFC2136 fixes [WIP] RFC2136 fixes Aug 16, 2018
@Habbie
Copy link
Member Author

Habbie commented Aug 16, 2018 •
edited

1dyndns-correct-zone also passes on master, so it's a bad test.

@Habbie
Copy link
Member Author

Habbie commented Aug 17, 2018

1dyndns-correct-zone also passes on master, so it's a bad test.

Fixed the test and reordered the commits to make that easy to verify.

@pieterlexis pieterlexis self-requested a review August 17, 2018 08:34
@Habbie Habbie changed the title [WIP] RFC2136 fixes RFC2136 fixes Aug 17, 2018
@pieterlexis
Copy link
Contributor

Tests broken?

[2018-08-17 11:41:35] verify-dnssec-zone
[2018-08-17 11:41:35] --- ./tests/verify-dnssec-zone/expected_result	2018-08-17 11:19:26.567911034 +0000
[2018-08-17 11:41:35] +++ ./tests/verify-dnssec-zone/real_result	2018-08-17 11:41:35.407705482 +0000
[2018-08-17 11:41:35] @@ -41,7 +41,7 @@
[2018-08-17 11:41:35]  RETVAL: 0
[2018-08-17 11:41:35]  
[2018-08-17 11:41:35]  --- named-checkzone sub.test.dyndns
[2018-08-17 11:41:35] -zone sub.test.dyndns/IN: loaded serial 2018081702 (DNSSEC signed)
[2018-08-17 11:41:35] +zone sub.test.dyndns/IN: loaded serial 2012060701 (DNSSEC signed)
[2018-08-17 11:41:35]  OK
[2018-08-17 11:41:35]  RETVAL: 0
[2018-08-17 11:41:35] ****

@Habbie Habbie changed the title RFC2136 fixes [WIP] RFC2136 fixes Aug 17, 2018
@Habbie Habbie changed the title [WIP] RFC2136 fixes RFC2136 fixes Aug 18, 2018
@Habbie
Copy link
Member Author

Habbie commented Aug 18, 2018

Tests broken?

Yes, fixed now!

rgacogne
rgacogne approved these changes Aug 20, 2018
View reviewed changes
Copy link
Member

@rgacogne rgacogne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, very nice job with the tests!

pdns/rfc2136handler.cc Outdated
@@ -33,7 +33,7 @@ int PacketHandler::checkUpdatePrerequisites(const DNSRecord *rr, DomainInfo *di)

bool foundRecord=false;
DNSResourceRecord rec;
di->backend->lookup(QType(QType::ANY), rr->d_name);
di->backend->lookup(QType(QType::ANY), rr->d_name, 0, di->id);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a nit but I'd rather see nullptr than 0 here and in the following calls to lookup().

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pushed

pieterlexis
pieterlexis approved these changes Aug 20, 2018
View reviewed changes
Copy link
Contributor

@pieterlexis pieterlexis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice and easy fix :)

pieterlexis added a commit to pieterlexis/pdns that referenced this pull request Aug 20, 2018
@pieterlexis
Backport PowerDNS#6858
3c76855
@pieterlexis pieterlexis merged commit de9a4eb into PowerDNS:master Aug 21, 2018
pieterlexis added a commit to pieterlexis/pdns that referenced this pull request Aug 21, 2018
@pieterlexis
Backport PowerDNS#6858
c734f35
@pieterlexis pieterlexis mentioned this pull request Aug 21, 2018
Merged
8 tasks
@Habbie Habbie deleted the rfc2136-correct-zone branch August 22, 2018 17:59
@PowerDNS PowerDNS deleted a comment from savagegeek Aug 22, 2018
@Habbie Habbie mentioned this pull request Nov 20, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pieterlexis pieterlexis pieterlexis approved these changes

@rgacogne rgacogne rgacogne approved these changes

Assignees
No one assigned
Labels
auth defect
Projects
None yet
Milestone
auth-4.1.x
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Habbie @pieterlexis @rgacogne